Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[docs] Add docs for reworked Fivetran API #26026

Open
wants to merge 7 commits into
base: maxime/move-fivetran-docs-to-legacy-page
Choose a base branch
from

Conversation

maximearmstrong
Copy link
Contributor

@maximearmstrong maximearmstrong commented Nov 20, 2024

Summary & Motivation

As title

How I Tested These Changes

See preview

Copy link
Contributor Author

maximearmstrong commented Nov 20, 2024

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@maximearmstrong maximearmstrong force-pushed the maxime/move-fivetran-docs-to-legacy-page branch from f42cebd to c01caf8 Compare November 22, 2024 13:55
@maximearmstrong maximearmstrong force-pushed the maxime/move-fivetran-docs-to-legacy-page branch from c01caf8 to 9e707b8 Compare November 22, 2024 14:15
@maximearmstrong maximearmstrong force-pushed the maxime/move-fivetran-docs-to-legacy-page branch from 9e707b8 to d14c6d2 Compare November 22, 2024 14:19
@maximearmstrong maximearmstrong force-pushed the maxime/move-fivetran-docs-to-legacy-page branch from d14c6d2 to ad961a8 Compare November 22, 2024 14:22
@maximearmstrong maximearmstrong force-pushed the maxime/move-fivetran-docs-to-legacy-page branch from ad961a8 to 66a92e1 Compare November 25, 2024 13:06
@maximearmstrong maximearmstrong force-pushed the maxime/move-fivetran-docs-to-legacy-page branch from 66a92e1 to 7cc3bc7 Compare November 26, 2024 23:08
@maximearmstrong maximearmstrong force-pushed the maxime/move-fivetran-docs-to-legacy-page branch from a7794fb to 3dd795c Compare November 27, 2024 19:28
@maximearmstrong maximearmstrong marked this pull request as ready for review November 27, 2024 19:30
@maximearmstrong maximearmstrong self-assigned this Nov 27, 2024
@neverett neverett added the docs-to-migrate Docs to migrate to new docs site label Dec 2, 2024
yield from fivetran.sync_and_poll(context=context)


# Alternatively, when creating all assets definitions for the Fivetran workspace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grammar here seems a bit off


fivetran_specs = load_fivetran_asset_specs(fivetran_workspace)
defs = dg.Definitions(
assets=[*fivetran_specs], resources={"fivetran": fivetran_workspace}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be able to just fivetran_specs, no need for splat operator right

)


# Creating assets definition for a given connector using the `@fivetran_assets` decorator
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment is a bit redundant

Copy link
Contributor

@dpeng817 dpeng817 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python files have hyphenated names, let's fix that.

Additionally, I think the structuring here is a bit weird. We should probably lead with build_fivetran_assets_... since that's the most straightforward way. Then, you can talk about "customizing execution functionality" and go through using the other APIs.

A few additional comments as well.


By default, Dagster will generate asset specs for each Fivetran asset and populate default metadata. You can further customize asset properties by passing a custom <PyObject module="dagster_fivetran" object="DagsterFivetranTranslator" /> subclass to the <PyObject module="dagster_fivetran" method="load_fivetran_asset_specs" /> function, the <PyObject module="dagster_fivetran" method="fivetran_assets" /> decorator or the <PyObject module="dagster_fivetran" method="build_fivetran_assets_definitions" /> factory.

```python file=/integrations/fivetran/customize-fivetran-asset-defs.py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole code snippet feels really long, can we make use of startbefore and endafter to trim this down to only the relevant bits?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-to-migrate Docs to migrate to new docs site
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants